-
Notifications
You must be signed in to change notification settings - Fork 190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kie-issues#2659: [SonataFlow] Migrate DBMigration Image from SonataFlow operator repository to kie-tools #2697
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments to adjust the PR. Thank you!
packages/kogito-db-migrator-tool-image/cekit/modules/kogito-postgres-db-migration-deps/install
Outdated
Show resolved
Hide resolved
...ges/kogito-db-migrator-tool/src/main/cekit/modules/kogito-postgres-db-migration-deps/install
Outdated
Show resolved
Hide resolved
...ogito-db-migrator-tool/src/main/cekit/modules/kogito-postgres-db-migration-deps/migration.sh
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the same directory structure we already have in the other images: https://github.com/apache/incubator-kie-tools/tree/main/packages/kogito-data-index-ephemeral-image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more inline comments. Thanks for the changes so far @rhkp!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are almost there!
By the way, have you successfully built this image locally using this package?
packages/kogito-db-migrator-tool-image/resources/build-container-image.sh
Outdated
Show resolved
Hide resolved
...ogito-db-migrator-tool-image/resources/modules/kogito-postgres-db-migration-deps/module.yaml
Outdated
Show resolved
Hide resolved
packages/kogito-db-migrator-tool-image/resources/kogito-db-migrator-tool-image.yaml
Outdated
Show resolved
Hide resolved
@rhkp Not sure this is WIP, but having these new env vars on |
Hi @tiagobento, |
packages/kogito-db-migrator-tool-image/resources/build-container-image.sh
Outdated
Show resolved
Hide resolved
packages/kogito-db-migrator-tool-image/resources/kogito-db-migrator-tool-image.yaml
Outdated
Show resolved
Hide resolved
packages/kogito-db-migrator-tool-image/resources/kogito-db-migrator-tool-image.yaml
Outdated
Show resolved
Hide resolved
...ogito-db-migrator-tool-image/resources/modules/kogito-postgres-db-migration-deps/module.yaml
Show resolved
Hide resolved
packages/kogito-db-migrator-tool-image/resources/kogito-db-migrator-tool-image.yaml
Outdated
Show resolved
Hide resolved
2a46afe
to
5f346a0
Compare
7d4cdb0
to
061ff1c
Compare
...ges/kogito-db-migrator-tool-image/resources/incubator-kie-kogito-db-migrator-tool-image.yaml
Show resolved
Hide resolved
...ogito-db-migrator-tool-image/resources/modules/kogito-postgres-db-migration-deps/module.yaml
Outdated
Show resolved
Hide resolved
61ef27f
to
35f182e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rhkp , I have added some comments, mostly nitpicks.
Great work!
packages/kogito-db-migrator-tool/src/main/resources/application.properties
Outdated
Show resolved
Hide resolved
packages/kogito-db-migrator-tool/src/main/resources/application.properties
Outdated
Show resolved
Hide resolved
...-migrator-tool/src/test/java/org/kie/kogito/postgresql/migrator/DBConnectionCheckerTest.java
Outdated
Show resolved
Hide resolved
...-migrator-tool/src/test/java/org/kie/kogito/postgresql/migrator/DBConnectionCheckerTest.java
Outdated
Show resolved
Hide resolved
...-migrator-tool/src/test/java/org/kie/kogito/postgresql/migrator/DBConnectionCheckerTest.java
Outdated
Show resolved
Hide resolved
- Start up a clean container with: | ||
|
||
```bash | ||
docker run docker.io/apache/incubator-kie-kogito-service-db-migration-postgresql:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have built the image locally and executed the migration, it worked well 💪 :
see: https://gist.github.com/wmedvede/549ecb613e2c27d09f4978624c43253b
The only weird thing is that in the logs I can see this:
2024-12-17 17:24:01,255 WARN [io.qua.run.con.ConfigRecorder] (main) Build time property cannot be changed at runtime:
- quarkus.platform.version is set to '3.8.6' but it is build time fixed to '999-SNAPSHOT'. Did you change the property quarkus.platform.version after building the application?
2024-12-17 17:24:01,476 INFO [io.quarkus] (main) sonataflow-db-migrator on JVM (powered by Quarkus 3.8.6) started in 0.443s.
The quarkus.platform.version should never have been 999-SNAPSHOT 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This indicates some issue with the env you may have used. May be some old image is lingering there. The latest logs can be found here and it nowhere refers to 999-SNAPSHOT
.
https://gist.github.com/rhkp/bbee2c6654068ad7009cff1319280312
I built the image using:
pnpm --stream=true -F kogito-db-migrator-tool-image... build:prod
And executed with (please change params based on your needs)
docker run --network=host --env MIGRATE_DB_DATAINDEX=true --env QUARKUS_DATASOURCE_DATAINDEX_JDBC_URL=jdbc:postgresql://127.0.0.1:5432/di --env QUARKUS_DATASOURCE_DATAINDEX_USERNAME=postgres --env QUARKUS_DATASOURCE_DATAINDEX_PASSWORD=postgres --env QUARKUS_FLYWAY_DATAINDEX_SCHEMAS=di --env MIGRATE_DB_JOBSSERVICE=true --env QUARKUS_DATASOURCE_JOBSSERVICE_JDBC_URL=jdbc:postgresql://127.0.0.1:5432/js --env QUARKUS_DATASOURCE_JOBSSERVICE_USERNAME=postgres --env QUARKUS_DATASOURCE_JOBSSERVICE_PASSWORD=postgres --env QUARKUS_FLYWAY_JOBSSERVICE_SCHEMAS=js docker.io/apache/incubator-kie-kogito-db-migrator-tool:main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the latest commits
I have deleted previous image
I have re-build this image
I can still see:
2024-12-18 11:39:32,878 WARN [io.qua.run.con.ConfigRecorder] (main) Build time property cannot be changed at runtime:
- quarkus.platform.version is set to '3.8.6' but it is build time fixed to '999-SNAPSHOT'. Did you change the property quarkus.platform.version after building the application?
https://gist.github.com/wmedvede/aff0a7afa3ed948bce894fdc6e9f5854
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I found, when building the DB Migrator Image:
if the QUARKUS_PLATFORM_VERSION
env var in terminal is not set, the 999-SNAPSHOT version appears.
However when the QUARKUS_PLATFORM_VERSION
env var in terminal is set when the image is built with a value such as 3.8.6
this does not appear.
However not sure where and why the 999-SNAPSHOT
version is being specifically picked up, as the DB migrator tool and/or image do not specifically use these versions.
A quick search of this string in sources shows 999-SNAPSHOT
is not used in the tool e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wmedvede and @ricardozanini,
I spent more time on this one.
EXECUTE DB MIGRATOR JAVA JAR IN TERMINAL(WITHOUT IMAGE I MEAN)
- The execution of the DB migrator with
java -jar target/quarkus-app/quarkus-run.jar
gives identical results whether the export QUARKUS_PLATFORM_VERSION=3.8.6 variable is set or not. - Meaning in any of the situation above I am not seeing 999-SNAPSHOTS log.
- Please note the WARN message for 999-SNAPSHOT comes from
[io.qua.run.con.ConfigRecorder]
package, when we are using image and I do not at all see this package in the logs in these scenarios.
COMPARE DB MIGRATOR IMAGE build FOLDERS
- I compared TWO directories one built with env vars set and another without setting one.
- The files are identical in this case too.
- And not sure where the 999-SNAPSHOT appears from in the logs when the image is built without setting export QUARKUS_PLATFORM_VERSION=3.8.6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wmedvede and @ricardozanini, I spent more time on this one.
EXECUTE DB MIGRATOR JAVA JAR IN TERMINAL(WITHOUT IMAGE I MEAN)
- The execution of the DB migrator with
java -jar target/quarkus-app/quarkus-run.jar
gives identical results whether the export QUARKUS_PLATFORM_VERSION=3.8.6 variable is set or not.- Meaning in any of the situation above I am not seeing 999-SNAPSHOTS log.
- Please note the WARN message for 999-SNAPSHOT comes from
[io.qua.run.con.ConfigRecorder]
package, when we are using image and I do not at all see this package in the logs in these scenarios.COMPARE DB MIGRATOR IMAGE build FOLDERS
- I compared TWO directories one built with env vars set and another without setting one.
- The files are identical in this case too.
- And not sure where the 999-SNAPSHOT appears from in the logs when the image is built without setting export QUARKUS_PLATFORM_VERSION=3.8.6.
I have built again locally and executed both scenarios:
java -jar target/quarkus-app/quarkus-run.jar:
Produce NO Warnings:
see full execution log here: https://gist.github.com/wmedvede/b3631a99b041bd66afdda5e9cb3120b1
--/ __ / / / / _ | / _ / /// / / / __/
-/ // / // / __ |/ , / ,< / // /\ \
--___// |//|//||_//
2024-12-20 13:03:31,609 INFO [io.quarkus] (main) kogito-db-migrator-tool 0.0.0 on JVM (powered by Quarkus 3.8.6) started in 0.469s.
2024-12-20 13:03:31,615 INFO [io.quarkus] (main) Profile prod activated.
2024-12-20 13:03:31,615 INFO [io.quarkus] (main) Installed features: [agroal, cdi, flyway, jdbc-postgresql, narayana-jta, smallrye-context-propagation]
2024-12-20 13:03:31,626 INFO [org.kie.kog.pos.mig.DBConnectionChecker] (main) Checking DB connection: jdbc:postgresql://localhost:5432/data-index - success
Image execution: docker run docker.io/apache/incubator-kie-kogito-db-migrator-tool:main bla bla
Produce warnings
see full execution log here: https://gist.github.com/wmedvede/f7ad4a7ca9df7201438ab053b96dd38b
--/ __ / / / / _ | / _ / /// / / / __/
-/ // / // / __ |/ , / ,< / // /\ \
--___// |//|//||_//
2024-12-20 12:07:29,367 WARN [io.qua.run.con.ConfigRecorder] (main) Build time property cannot be changed at runtime:
- quarkus.platform.version is set to '3.8.6' but it is build time fixed to '999-SNAPSHOT'. Did you change the property quarkus.platform.version after building the application?
2024-12-20 12:07:29,599 INFO [io.quarkus] (main) kogito-db-migrator-tool 0.0.0 on JVM (powered by Quarkus 3.8.6) started in 0.476s.
@rhkp @ricardozanini I conclude that the issue is introduced during the image build process, while the quarkus-run.jar is fine. Apart of doing the review, I have spent some time trying to see where issue comes from, but couldn't find.
Maybe someone else with fresh eyes can take a look and identify, I think it should be a silly nitpick.
@rgdoliveira @domhanak or maybe @fantonangeli , would you mind guys take look?
f588a53
to
8252831
Compare
Another general comment that I have just noted:
however, the package is: kogito-db-migrator-tool For me it doesn't look consistent 🤔 wdyt? Then, in the case of the image: The package is: kogito-db-migrator-tool-image and we produce: apache/incubator-kie-kogito-db-migrator-tool, looks consistent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a couple of new comments, take a look please.
...ogito-db-migrator-tool-image/resources/modules/kogito-postgres-db-migration-deps/module.yaml
Outdated
Show resolved
Hide resolved
8769274
to
c21904d
Compare
c21904d
to
911dc62
Compare
1f6ab2a
to
911dc62
Compare
Closes #2659
Add kogito-db-migrator-tool and kogito-db-migrator-tool-image packages for use by the SonataFlow Operator.